add fields to configure plugin conda executable and env paths#2654
add fields to configure plugin conda executable and env paths#2654emlys wants to merge 17 commits into
Conversation
| fs.writeFileSync(baseEnvYMLPath, baseEnvYMLContents); | ||
| await spawnWithLogging(micromamba, [ | ||
| 'create', '--yes', '--prefix', `"${baseEnvPrefix}"`, | ||
| 'env', 'create', '--yes', '--prefix', `"${baseEnvPrefix}"`, |
There was a problem hiding this comment.
It seems that "env create" rather than just "create" is necessary for compatibility with conda.
emilyanndavis
left a comment
emilyanndavis
left a comment
There was a problem hiding this comment.
Thanks, @emlys. I found a bug in the browse button behavior, and I left some other (minor) suggestions as well.
Beyond those code-specific comments, a couple of other things came to mind when I was trying out the "Configure conda executable" UI:
- It's not immediately clear that the path to the executable is actually the path to the directory that contains the executable, not the full path including the name of the executable (at least I'm assuming that's the case, since I was able to select only folders, not actual executable files). I wonder if there's a way to elegantly include this detail in the description, and/or add an example? I expect many people who use this option will use the
browsebutton and figure it out from context (like I did), but I wonder what would happen if someone directly entered the full path including the name of the executable. - The default value,
micromamba, could be misleading, since it's not obviously a path. I can imagine someone entering simplycondaand ending up disappointed. Is the default value in fact a path? If so, is there a way to make that more obvious (such as expanding it to its full path)? If not, could we display the path? Or perhaps expand the display text to something likeInVEST-bundled micromamba (default)so it's abundantly clear it's not merely the name of the package manager?
| <Button | ||
| aria-label="browse for conda executable" | ||
| className="browse-button ms-1 me-1" | ||
| id="browse-conda-button" |
There was a problem hiding this comment.
No need for an id here unless you are referencing it elsewhere (and it doesn't look like you are). If you want a handle for testing, it can be convenient to add a data- attribute that is clearly there for testing purposes. (For example, data-testid, which we use in a couple of components, automatically works with the DOM Testing Library's findByTestId query.)
| <Button | ||
| aria-label="browse for env" | ||
| className="browse-button ms-1 me-2" | ||
| id="browse-env-button" |
There was a problem hiding this comment.
No need for an id here either.
There was a problem hiding this comment.
…unless that id ends up being referenced by the click handler, in which case, make sure each instance of this button has a unique id (e.g., including the relevant plugin ID).
| className="browse-button ms-1 me-2" | ||
| id="browse-env-button" | ||
| variant="outline-dark" | ||
| onClick={selectDirectory} |
There was a problem hiding this comment.
I couldn't figure out why the browse buttons didn't seem to be working for me, until I looked more closely at the code. Every browse button in the modal is calling selectDirectory on click… and selectDirectory always populates the path field in the 'Add a plugin' form! So when I use a browse button to choose a conda path or a plugin env, my selection lands in the wrong form. You may want to write separate functions, or it might make sense to refactor selectDirectory to take another argument or perhaps parse data attached to the event.
There was a problem hiding this comment.
Some automated tests might be helpful here, too! 😃
Co-authored-by: Emily Davis <emilyanndavis@gmail.com>
Co-authored-by: Emily Davis <emilyanndavis@gmail.com>
Co-authored-by: Emily Davis <emilyanndavis@gmail.com>
Co-authored-by: Emily Davis <emilyanndavis@gmail.com>
|
Clearly I forgot to try out the file chooser dialog for the conda executable! It is supposed to select a file, not a directory. When the binaries are built, the default executable will look like an actual path, something like I don't know that the file chooser is very useful here, I imagine the typical use would be replacing the micromamba path with |
2 participants
Description
Fixes #2645, fixes #2646
Add an interface in the "Manage Plugins" modal to change the configured conda executable and plugin environment paths (equivalent to the
micromambaandplugins.<plugin ID>.envproperties inconfig.json).I added
defaultMicromambaandplugins.<plugin ID>.defaultEnvproperties to theconfig.jsonto track the original values so that they can be reset if needed.Needed to mock the plugin data returned from
GET_SETTINGin a couple more tests.Checklist